Skip to content

fix(schemas/yazi.json): set minimum of 3 for offset height#47

Open
uncenter wants to merge 1 commit intomainfrom
fix/offset-min-max
Open

fix(schemas/yazi.json): set minimum of 3 for offset height#47
uncenter wants to merge 1 commit intomainfrom
fix/offset-min-max

Conversation

@uncenter
Copy link
Member

@AminurAlam I think we can't use a reference to a type and override fields like min/max from that type at the same time. I've removed the minimum of 0 on the 3rd item in the offset since that is the default for u16, and then for the fourth item / offset height I switched off using u16 entirely and just manually set the max to the u16 max value. This now appears to properly fail if the height offset is below 3.

@AminurAlam
Copy link
Contributor

overriding it worked fine for me when testing

i'm against using u16 because yazi parses the value as i16 and errors out if its any higher than that
invalid value: integer 60000 expected i16

if you still prefer to not override it i would suggest doing "type": "integer", "minimum": 0, "maximum": 32767 like in pr #45

@uncenter
Copy link
Member Author

overriding it worked fine for me when testing

Did you test it when using the i16/u16 definitions? In #45 you didn't use u16 at all, I can't get the current state on the main branch to invalidate a value of 2 in the 4th spot of offset array.

@uncenter
Copy link
Member Author

i'm against using u16 because yazi parses the value as i16 and errors out if its any higher than that

Yeah I'm not too sure what's going on with it, there's some casting of values to u16 and I mean the default value for the offset width is u16 max. cc @sxyazi

@AminurAlam
Copy link
Contributor

AminurAlam commented Jul 10, 2025

Did you test it when using the i16/u16 definitions?

oops i missed that, also now that i look at it i used u16 in pr#46 by mistake

this is what i would recommend now:

  1. i16
  2. i16
  3. integer, min=0
  4. integer, min=3

@uncenter
Copy link
Member Author

That's what this is with the exception that 3 and 4 don't have an upper bound.

@sxyazi
Copy link
Member

sxyazi commented Jul 10, 2025

Yes, it's a bit inconsistent here. I did a blame and found that the initial implementation used a Vec<i16>, and it was refactored to [i16; 4] two days ago.

I think at that time I wasn't know that serde supports deserialization of static arrays (i.e. [i16, i16, u16, u16]), so I used Vec<i16> to accept all i16 values and then cast width and height to u16 from i16 manually.

However, I plan to refactor these properties from arrays to dictionaries, for example:

- trash_offset = [ 0, 0, 70, 20 ]
+ trash_offset = { x = 0, y = 0, width = 70, height = 20 }

to provide better readability, and to support default values so that 0 can be omitted:

- trash_offset = { x = 0, y = 0, width = 70, height = 20 }
+ trash_offset = { width = 70, height = 20 }

Maybe we could hold off on this until then to avoid rework? WDYT?

@uncenter
Copy link
Member Author

Maybe we could hold off on this until then to avoid rework? WDYT?

Sure that sounds good! I like the dictionary idea, much better with those benefits you mention and easier for us over here ahah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants